-
Notifications
You must be signed in to change notification settings - Fork 3
[ingress][torch-mlir][RFC] Initial version of fx-importer script using torch-mlir #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…h-mlir Signed-off-by: dchigarev <[email protected]>
Signed-off-by: dchigarev <[email protected]>
ingress/Torch-MLIR/generate-mlir.sh
Outdated
@@ -1,28 +1,54 @@ | |||
#!/usr/bin/env bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
example:
./generate-mlir.sh -m torchvision.models:resnet18 -S 1,3,224,224,float32 -o now.mlir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we include this in the repo somewhere. While the repo doesn't really have tests yet, and certainly no CI, having a working example of the cmdline interface to this is helpful (or even necessary).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to also have an in-repo example of how to invoke the conversion from inside a user script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added examples/
folder with several use cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dchigarev for making progress on PyTorch ingress!
In general it all seems to make sense to me! My comments are on relatively small matters.
Having said that, I would say the majority of the PR is on enabling the cmdline interface, which I expect to also be the most contentious. Personally, I am not a fan of such interfaces and prefer the scripting approach. If other people are in favour though, I am not opposed for the code to be included.
Do you happen to have examples of similar cmdline interfaces being used for enabling PyTorch lowerings in other projects?
ingress/Torch-MLIR/generate-mlir.sh
Outdated
@@ -1,28 +1,54 @@ | |||
#!/usr/bin/env bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we include this in the repo somewhere. While the repo doesn't really have tests yet, and certainly no CI, having a working example of the cmdline interface to this is helpful (or even necessary).
ingress/Torch-MLIR/generate-mlir.py
Outdated
|
||
entrypoint = load_callable_symbol(args.model_entrypoint) | ||
|
||
model = entrypoint(*eval(args.model_args), **eval(args.model_kwargs)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes - that's quite some eval
.
I guess if we are to have a cmdline interface, there's not much to be done about it.
ingress/Torch-MLIR/generate-mlir.sh
Outdated
@@ -1,28 +1,54 @@ | |||
#!/usr/bin/env bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to also have an in-repo example of how to invoke the conversion from inside a user script.
ingress/Torch-MLIR/generate-mlir.sh
Outdated
echo "Generating MLIR for model '$MODEL' with dialect '$DIALECT'..." | ||
python $SCRIPT_DIR/generate-mlir.py --model "$MODEL" --dialect "$DIALECT" | ||
echo "Generating MLIR for model entrypoint '$MODEL' with dialect '$DIALECT'..." | ||
python "$SCRIPT_DIR/generate-mlir.py" "${args[@]}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this entire bash script be folded into the Python script?
At this point I do not see the .sh giving much value. I guess it is necessary for entering the virtualenv, otherwise it's just a wrapper, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A brainwave: could the python script exec
itself after it has set up the right environment variables, i.e. the ones which correspond to entering the virtualenv? Or more hacky: os.system("source .../bin/activate; python "+__file__.__path__)
in case we detect not being in the venv, e.g. due to imports failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the idea for a python script to deduce whether it was launched in a proper env and modifying it. I would say a user should be responsible for setting up a proper env before launching the python script (they can always call a bash version of the script that handles venvs for them).
Simplified the generate-mlir.sh
script so that it only activates venv and forwards all the arguments to the python script.
Signed-off-by: dchigarev <[email protected]>
Signed-off-by: dchigarev <[email protected]>
@rolfmorel thanks for your time and feedback! No, I haven't seen such cmdline approach anywhere (I wasn't looking to deep though). On the surface of IREE's and Blade's documentation I could only found the user-script approach. So even if they have a cmdline option, they don't seem to promote it very well. |
This is great, thank you so much for working on this 🙏🏻 I have a few high-level suggestions. Keep this PR simple and restrict to the required minimum. The cmdline interface looks complex and is merely a "wrapper" for the script logic. We can't avoid having a script, but we can avoid the cmdline interface. And, with a complex cmdline interface like this, I would wrap it into yet another script. My suggestion - drop the interface for now. This will allow us to focus on the core logic instead. Consistent filenames and hyphenation.
Use doctoring consistently. Lets use (function + module) docstrings consistently (instead of mixing docstring and plain Python comments starting with Do we need all the Bash scripts? There's seems to be a fair bit of duplication, e.g. Naming.
IIUC, While Final thoughts. Really fantastic to see this, just a bit concerned that this PR is trying to achieve too many things in one go. I recommend trimming it - I'd much rather focus on the core part and also make sure that we establish a consistent way of naming, structuring and implementing things. I've some other, more specific comments inline. Thanks again for working on this! 🙏🏻 |
|
||
|
||
def generate_mlir(model, sample_args, sample_kwargs=None, dialect="linalg"): | ||
# Convert the Torch model to MLIR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use docstrings consistently throughout this project?
@@ -0,0 +1,16 @@ | |||
#!/usr/bin/env bash | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set -euo pipefail |
@@ -0,0 +1,16 @@ | |||
#!/usr/bin/env bash | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DOCUMENTME - what is the purpose of this script and how do I use it?
import argparse | ||
from export_lib.export import load_torch_model, generate_sample_args, generate_mlir | ||
|
||
# Parse arguments for selecting which model to load and which MLIR dialect to generate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use docstring consistently.
@@ -0,0 +1,98 @@ | |||
#!/usr/bin/env python3 | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DOCUMENTME - what is the purpose of this script and how do I use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the main purpose of this file is to export to MLIR, then main
-> export
? or export-torch-to-mlir
? Or some combination?
--out-mlir res.mlir | ||
``` | ||
|
||
Look into `examples/` folder for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, this script won't generate MLIR - it will export to MLIR? Could you rename accordingly?
Also, why do we need a Bash wrapper for the Python script? Why isn't Python enough?
Building wrapper scripts around torch-mlir is not scalable at all. torch-mlir is not a library to build things with, not a tool to build scripts around. The proper way of doing this is shipping fx_importer as part of bindings: #3 (ready for review) and then building export over it and ship it as part of the python package. I'm going to send a pr on building an aot export for torch and onnx around that today to give an idea of how it should be done. |
This PR modifies the
torch-mlir/generate-mlir.py -> torch-mlir/py_src/main.py
script to make it actually work. The script is now capable of converting a pytorch'snn.Model
to a mlir module.How pytorch models are usually being distributed
Usually when someone wants to download a pretrained model they end up with two files:
.pth or .pt
)Then the model's usage in a user's script would look like this:
In order to "load" a pytorch model to the export script we need to accept its state and a python's entrypoint that instantiates the models class. Example:
There's no trivial way to automatically deduce which arguments (tensor shapes) a model expects, so
torch-mlir
(and so our scripts) requires a "sample argument" which is basically an empty tensor(s) that has a proper shape and a dtype. Example:The script may also take positional and keyword arguments to pass to the model's instantiation function, as well as a custom function to generate "sample" models arguments.
The full list script's of arguments is the following:
--model-entrypoint
(required) Path to the model entrypoint, e.g. 'torchvision.models:resnet18' or '/path/to/model.py:build_model'--model-state-path
(optional since an entry-point function may already setup proper state) Path to a state file of the Torch model (usually has .pt or .pth extension).--model-args (default: "[]")
(optional) Positional arguments to pass to the model's entrypoint--model-kwargs (default: "{}")
(optional) Keyword arguments to pass to the model's entrypoint--sample-shapes
(optional) Tensor shapes/dtype that the 'forward' method of the model will be called with. Must be specified if '--sample-fn' is not given.--sample-fn
(optional) Path to a function that generates sample arguments for the model's 'forward' method.--dialect
{"torch", "linalg", "stablehlo", "tosa"}--out-mlir
(optional) Path to save the generated MLIR moduleThe need to accept a lot of python objects (entrypoint, args, kwargs) as a string argument makes the script fragile and error prone. There seems to be no other way though if we want the export script to be responsible for "loading" and exporting models.
Is there an alternative?
IREE
andBlade
went a different way and make a user responsible for instantiating a model in their own script. A user then calls an export function (provided by iree or blade library) on their instantiated model.An example of a user's script to convert their torch model to mlir using iree
An example from iree's docs
Lighthouse's export script can also be used this way. A user simply needs to import the
generate_mlir
function and pass their model there: